Skip to content

Overhaul spawnveg to use clone(2) and fix many related spawning bugs #888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

JohnoKing
Copy link

This commit is a rewrite of #881. The previous pull request didn't address lingering bugs with E2BIG handling and file descriptor leaks, proving unsatisfactory. After support for direct usage of clone was added to spawnveg, the previous fixes for the posix_spawn implementation wound up unused and were scrapped (although those changes do work just fine and can be readded if that's desirable).

Alterations:

  • Add a version of spawnveg that creates a new process with clone(2) outright. This method is generally faster than using posix_spawn(3), and is also more portable (because Linux with musl libc and NetBSD both provide clone, but not posix_spawn_file_actions_addtcsetpgrp_np). This code was initially based on the _real_vfork code previously removed in ef9a5c7 (unlike vfork(2), there is no problem with doing stuff prior to execve for clone with CLONE_VM | CLONE_VFORK).
    • Remove the now unused code implementing tcsetpgrp via posix_spawn_file_actions_addtcsetpgrp_np(). This code was only ever used with glibc 2.35+. (Not only that, but the only other platform that ever considered implementing a similar function was NetBSD1, where it appears to have been abandoned. This is ironic considering ksh no longer uses posix_spawn on NetBSD as of this commit.) This code may be worth revisiting if io_uring_spawn()2 is ever actualized (that project appears to be rather inactive though).
      Demonstration of the (small) performance improvement garnered from using clone directly rather than via posix_spawn:
time ksh-posix-spawn -c 'for i in {1..15000}; do /bin/true; done'

real	0m05.461s
user	0m03.646s
sys	0m01.869s

time ksh-clone -c 'for i in {1..15000}; do /bin/true; done'

real	0m05.373s
user	0m03.629s
sys	0m01.738s
  • Bugfix: Don't set the terminal signals to their defaults in the parent prior to calling spawnveg. This is the primary cause of a lockup in the pty tests which can be reproduced as follows:
$ exec ksh
$ bin/shtests pty 2>/dev/null
^C ^C ^C (SIGINT doesn't work anymore and may segfault)

The correct way to go about dealing with SIGT* is to set those to SIG_DFL in the child process3.

  • Added support for tcsetpgrp to the fork fallback in spawnveg. Some form of this appears to have already been attempted in AT&T olden times, but that old version was broken and needed bugfixes desperately.
    • If the child needs tcsetpgrp, block the terminal signals in the parent process via sigcritical().
  • Now that the fork fallback for spawnveg works correctly in interactive terminals, prefer that to the sh_fork() codepath on operating systems without clone support. Even though the underlying system call is still ultimately fork, the sh_ntfork codepath is faster than the traditional sh_fork codepath. Benchmark:
# Note: This benchmark was done on FreeBSD, though the results can be replicated elsewhere
$ time /tmp/ksh-devbranch -ic "for i in {1..10000}; do $(whence -p true); done"
     real       0m03.302s
     user       0m00.988s
     sys        0m02.320s
$ time /tmp/ksh-newspawnveg -ic "for i in {1..10000}; do $(whence -p true); done"
     real       0m03.160s
     user       0m01.187s
     sys        0m01.977s
  • To that end, split up the spawnveg implementations into spawnveg_fast and spawnveg_slow. Choose the appropriate one when spawnveg is called; this removes the need for the xec.c ifdef hackery.
    • Removed the ntfork_tcpgrp ifdefs from xec.c; spawnveg can handle it by itself now.
  • Bugfix: With the spawnveg_fast and spawnveg_slow innovation, spawnveg now always has support for setsid. It'll fallback to fork() if POSIX_SPAWN_SETSID and clone() aren't available.
  • Bugfix: For the posix_spawn version of spawnveg, the flags should be of the short type pursuant to the POSIX specification.
  • Optimization: Use pipe2 in the fork fallback for spawnveg when it's available to avoid two fcntl syscalls.
  • Updated the spawnveg documentation to reflect the new changes.
  • _spawnveg(): Restore the terminal process group immediately after any relevant failed spawn attempt, rather than only in sh_ntfork().
    • Added a regression test to pty.sh for a crash that occurred because of the erroneous tcsetpgrp placement.
      Reproducer that could cause ksh to prematurely exit (for Linux systems with glibc 2.35+):
$ $(type -p true) /ultralongpathname/{1..1000000}
/usr/bin/ksh: /usr/bin/true: /usr/bin/true: cannot execute [Argument list too long]
$ # Type any character
  • path_spawn(): Do not print an error message and longjmp away upon encountering E2BIG or some other error; let the calling functions take care of that.
  • path_exec(): Added handling for E2BIG.
  • Fixed a bug that caused sh_ntfork to leak file descriptors upon encountering an error (re: 8f848bc). (This bug/regression was encountered after fixing the bad tcsetpgrp usage.) Reproducer:
#!/bin/ksh
ls /proc/$$/fd
redirect 2>/dev/null
touch /tmp/file
for i in {1..20}; do
    command /tmp/file <(echo)
done
ls /proc/$$/fd
  • Added regression tests for the leak that use command and command -x.
    • Fixed an old regression test that was giving false negatives.
  • Clarified the intent of the sigcritical() nesting matches. The ksh2020 devs appeared to have been confused by this line of code, so some additional clarification explaining what it does should be helpful for posterity.
  • path.sh: Added tests for the exit status of commands run when forked with the & operator.

Footnotes

  1. https://blog.netbsd.org/tnf/entry/gsoc_reports_make_system_31

  2. https://lwn.net/Articles/908268/

  3. cf. _sh_fork()

… bugs

Alterations:
- Add a version of spawnveg that clones processes with clone(2)
  outright. This method is generally faster than using posix_spawn,
  and is also more portable (since Linux with musl libc *and* NetBSD
  both provide this function). The code was initially based on the
  _real_vfork spawnveg code previously removed in ef9a5c7 (unlike
  vfork(2), there is no problem with running code prior to execve
  for clone with CLONE_VM|CLONE_VFORK).
  - Remove the now dead code implementing tcsetpgrp via
    posix_spawn_file_actions_addtcsetpgrp_np(). This code was only
    ever used with glibc 2.35+. (Not only that, but the only other
    platform that ever considered implementing such a call was
    NetBSD[^1], where it appears to have been abandoned. This is
    ironic considering ksh no longer uses posix_spawn on NetBSD
    as of this commit.)
    This code may be worth visiting if io_uring_spawn()[^2] is ever
    actualized (that project appears to be quite inactive though).
- Bugfix: Don't set the terminal signals to their defaults
  in the parent prior to calling spawnveg. This is the primary
  cause of a lockup in the pty tests which can be reproduced
  as follows:
    $ exec ksh
    $ bin/shtests pty 2>/dev/null
    ^C ^C ^C (SIGINT doesn't work anymore and may segfault)
  The correct way to go about dealing with SIGT* is to set
  those to SIG_DFL in the child process (cf. _sh_fork()).
- Added support for tcsetpgrp to the fork fallback in spawnveg.
  Some form of this appears to have already been attempted in
  AT&T olden times, but that old version was broken and needed
  bugfixes desperately.
  - If the child needs tcsetpgrp, block the terminal signals
    in the parent process via sigcritical(). The posix_spawn
    version doesn't need this because posix_spawn will block
    signals automatically and therefore doesn't need sigcritical.
- Now that the fork fallback for spawnveg works correctly in
  interactive terminals, prefer that to the sh_fork() codepath
  on operating systems without posix_spawn tcsetpgrp support.
  Even though the underlying system call is still ultimately fork,
  the sh_ntfork() codepath is faster than the traditional sh_fork
  codepath. Benchmark:
     $ time /tmp/ksh-devbranch -ic "for i in {1..10000}; do $(whence -p true); done"
     real       0m03.302s
     user       0m00.988s
     sys        0m02.320s
     $ time /tmp/ksh-newspawnveg -ic "for i in {1..10000}; do $(whence -p true); done"
     real       0m03.160s
     user       0m01.187s
     sys        0m01.977s
- To that end, split up the spawnveg versions into spawnveg_fast
  and spawnveg_slow. Choose the appropriate one when spawnveg is
  called; this removes the need for the xec.c ifdef hackery.
  - Removed the ntfork_tcpgrp ifdefs from xec.c; spawnveg can
    handle it by itself now.
- Bugfix: With the spawnveg_fast and spawnveg_slow innovation,
  spawnveg now always has support for setsid. It'll fallback to
  fork if POSIX_SPAWN_SETSID and clone(2) aren't available.
- Bugfix: For the posix_spawn version of spawnveg, the flags should
  be of the short type pursuant to the POSIX specification.
- Optimization: Use pipe2 in the fork fallback for spawnveg when
  it's available to avoid two fcntl syscalls.
- Updated the spawnveg documentation to reflect the new changes.
- _spawnveg(): Restore the terminal process group immediately
  after any failed spawn attempt, rather than only in sh_ntfork().
  - Added a regression test for a crash that occurred because of
    the erroneous tcsetpgrp placement in sh_ntfork().
- path_spawn(): Do not print an error message and longjmp away
  upon encountering E2BIG or some other error; let the calling functions
  take care of that.
- path_exec(): Added handling for E2BIG.
- Fixed a bug that caused sh_ntfork to leak file descriptors
  upon encountering an error (re: 8f848bc). Reproducer:
     #!/bin/ksh
     ls /proc/$$/fd
     redirect 2>/dev/null
     touch /tmp/file
     for i in {1..20}; do
         command /tmp/file <(echo)
     done
     ls /proc/$$/fd
  - Added regression tests for the leak that use 'command' and
    'command -x'.
  - Fixed an old regression test that was giving false negatives.
- Clarified the intent of the sigcrit nesting matches. The ksh2020
  devs appeared to have been confused by this line, so some
  additional clarification explaining the intent should be helpful
  for posterity.
- path.sh: Added tests for the exit status of commands run when forked
  with the & operator.

[^1]: https://blog.netbsd.org/tnf/entry/gsoc_reports_make_system_31
[^2]: https://lwn.net/Articles/908268/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant